Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ONNX to Zhigh guided by cost model #2507

Closed

Conversation

AlexandreEichenberger
Copy link
Collaborator

Using Cedric's cost model to make a first approximation at only migrating to zAIU the ops that appears to be beneficial according to that cost model.

Enables using the -enable-zhigh-cost-model flag.

@AlexandreEichenberger AlexandreEichenberger marked this pull request as draft September 14, 2023 18:46
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
@AlexandreEichenberger AlexandreEichenberger marked this pull request as ready for review September 15, 2023 13:27
@AlexandreEichenberger
Copy link
Collaborator Author

@jenkins-droid test this please

@AlexandreEichenberger
Copy link
Collaborator Author

@jenkins-droid please test this

@tungld
Copy link
Collaborator

tungld commented Sep 15, 2023

FYI, I created a PR to place operations on devices by using device attribute #2510. If we use that, we don't need to pass the cost model to every legality check function.

Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
@chentong319
Copy link
Collaborator

chentong319 commented Sep 15, 2023

Since we are going to add different control on the conversion, I suggest that we clear up the program structure first.

  1. For a conversion, define an analysis set. Currently, there is only DimAnalysis.

  2. Decide which op will be converted with the current AddDynamicallyLegalOpFor<>(op, analysis_set, flags). Basically to implement addDynamicallLegal

  3. In the AddDynamicallyLegalOpFor (Actually target->addDynamicallyLegalOp<OP_TYPE>), we should unify the code for different Op and a common part as much as possible.
    First check whether it is legal for the conversion. This is the isSuitableForZDNN(op). If it returns false, this Op has to be legal. This Op can not be converted. I suggest change the name isSuitableForZDNN to isLegalForZDNN.
    Then use the analysis results (traverse the analysis set) to decide whether we want to convert the op. The analysis result can be generated with a global view, or from file or command line. Drawback is the possible overhead on Ops eliminated by other standard.
    Finally, check locally of the current Op to make decision.

  4. The rewriting rules decide ONLY HOW to convert, assuming that Ops have been checked and decided.

@AlexandreEichenberger @tungld Should we restructure the code before we add new code?

Signed-off-by: Alexandre Eichenberger <[email protected]>
@tungld
Copy link
Collaborator

tungld commented Sep 15, 2023

@chentong319 fyi #2510

@tong
Copy link

tong commented Sep 16, 2023

@tong fyi #2510

i am not the tong you are looking for.

@AlexandreEichenberger
Copy link
Collaborator Author

FYI, I created a PR to place operations on devices by using device attribute #2510. If we use that, we don't need to pass the cost model to every legality check function.

Sure, this PR can refactor the benefits as a separate function that can be separate from the legality check.

@AlexandreEichenberger
Copy link
Collaborator Author

I suggest that we clear up the program structure first.

You are making excellent points @chentong319, I will transform the code to separate benefit from validity (it was kind of separate already if we disable the benefit flag, but it is indeed better to have a stronger/clearer separation.

@tungld
Copy link
Collaborator

tungld commented Oct 3, 2023

@AlexandreEichenberger looks like this patch is replaced by #2534 that was landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants